-
Notifications
You must be signed in to change notification settings - Fork 325
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add "json"
destination, for JSON modules
#1691
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great modulo some nits.
I think we can count Chromium and WebKit as supportive based on statements in whatwg/html#7233, but I'll tag some people here just in case.
81b9d07
to
450893f
Compare
@nicolo-ribaudo are you planning to write tests for this and implementation bugs? |
cc/ @domenic |
Tests have been merged! |
Builds on whatwg/fetch#1691. Tests: web-platform-tests/wpt#41665. Fixes #7233.
The "Import Attributes" ECMAScript proposal has been updated to allow attributes to affect how modules are fetched, so that HTML can use them to set the proper destination when fetching CSS and JSON modules [1]. This has two major effects: - the `Accept` HTTP header is now specific to the module type, rather than simply being `*/*` - we use the relevant CSP policy rather than always script-src. This patch only implements this change for CSS modules. The change for JSON modules will come in a later patch, as it's more complex because it requires introducing a new fetch destination [2]. It passes the relevant wpt tests [3] when using --js-flags="--harmony_import_attributes. [1]: whatwg/html#9486 [2]: whatwg/fetch#1691 [3]: web-platform-tests/wpt#41665 Bug: 1491336 Change-Id: I4abf09dd828e5ded2b685be6da231c3fca90e19f Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4949956 Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org> Reviewed-by: Stephen Chenney <schenney@chromium.org> Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> Commit-Queue: Nicolò Ribaudo <nribaudo@igalia.com> Cr-Commit-Position: refs/heads/main@{#1249592}
This patch adds a new `json` fetch destination [fetch-spec-pr], that has the following characteristics: - it implies the `Accept: application/json,*/*;q=0.5` HTTP header; - it uses the `connect-src` CSP directive [csp-spec-pr]. This new destination is used when fetching JSON module scripts, using `import ... from "/data" with { type: "json" }` [html-spec-pr]. https://crrev.com/c/4949956 implements a similar change for CSS module scripts, but their implementation is simpler because the `style` destination already exists. This patch passes all the relevant WPT tests [wpt-pr] (when using --js-flags="--harmony_import_attributes), although I had to run them manually because they have not been merged yet. This patch does not add support for `<link rel="preload" as="json">`, which is also introduced by the linked fetch and HTML spec changes. [fetch-spec-pr]: whatwg/fetch#1691 [csp-spec-pr]: w3c/webappsec-csp#611 [html-spec-pr]: whatwg/html#9486 [wpt-pr]: web-platform-tests/wpt#41665 Bug: 1491336 Change-Id: I6661ddc9be04935e2ee760eb78d1060ae0192a55 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4955077 Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org> Reviewed-by: David Bertoni <dbertoni@chromium.org> Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org> Reviewed-by: Xinghui Lu <xinghuilu@chromium.org> Commit-Queue: Nicolò Ribaudo <nribaudo@igalia.com> Cr-Commit-Position: refs/heads/main@{#1249822}
The import attributes TC39 proposal has been updated so that HTML can use the expected type (unspecified,
json
,css
) to affect the fetch request. This PR introduces the"json"
destination, that will be used to fetchimport "..." with { type: "json" }
.Ref whatwg/html#9486 (comment), whatwg/html#7233
Accept
header for JSON modules with network imports denoland/deno#20866Accept
header for JSON modules with network imports nodejs/node#50116json
to possible fetch destinations mdn/content#29573(See WHATWG Working Mode: Changes for more details.)
Preview | Diff